demuxer: Fix demuxer assert logic and refactor FindFFmpeg#186
demuxer: Fix demuxer assert logic and refactor FindFFmpeg#186crmurillo wants to merge 4 commits intoKhronosGroup:mainfrom
Conversation
0d5c3c1 to
d0d2605
Compare
5420320 to
22626b5
Compare
7d59b8e to
0507fbc
Compare
0507fbc to
e89fe39
Compare
e89fe39 to
3f07740
Compare
…ions The module searched for FFmpeg libraries twice (once via a macro, once via pkg-config + find_library) and used "FFMPEG" as the pkg-config prefix, which meant pkg-config wrote over the module's own output variables like FFMPEG_FOUND and FFMPEG_LIBRARIES. The following changes were made: - Replace with a single search pass using PC_FFMPEG as the pkg-config prefix. - Remove avdevice/x264/x265 since nothing links against them. - Drop the outdated Win32 stdint.h check. - Make swscale optional. - Split FFMPEG_INCLUDE_DIR into per-component include dirs and centralize search paths to eliminate duplication across find_library calls. - Move Windows FFmpeg discovery into FindFFmpeg. - Unify FFmpeg linking in demo and test targets to use FFMPEG_LIB* variables on all platforms.
When FFmpeg is not found and no codec is explicitly passed, codecType defaults to VK_VIDEO_CODEC_OPERATION_NONE_KHR). In release builds, the asserts validating this are stripped, causing the demuxer to silently proceed with invalid parameters and ultimately fail with a misleading "Video decode queue family not supported by hardware/driver" error. Replace asserts with explicit validation that returns VK_ERROR_FORMAT_NOT_SUPPORTED with descriptive error messages. Also print a hint when FFmpeg demuxer support is not available, since this is the most common cause of the failure.
Updated the list of skipped tests for the proprietary AMD driver (Windows).
No source file references swscale, therefore, the optional lookup was dead code. Drop it from the module.
3f07740 to
f6e3ebe
Compare
| find_package(PkgConfig QUIET) | ||
| if(PKG_CONFIG_FOUND) | ||
| pkg_check_modules(FFMPEG QUIET | ||
| pkg_check_modules(PC_FFMPEG QUIET |
There was a problem hiding this comment.
the package config result should be high priority result setting all the necessary variable. I dont know if all the search and find are necessary afterwhile
| ) | ||
|
|
||
| # Find required libraries | ||
| find_library(FFMPEG_LIBAVCODEC_LIBRARIES |
There was a problem hiding this comment.
this is kind of confusing to have LIBARIES when we are searching for only one library. can we use FFMPEG_LIBAVCODEC_LIBRARY ?
| # Windows FFmpeg paths | ||
| if(WIN32) | ||
| set(FFMPEG_WIN32_PREBUILT_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/vk_video_decoder/bin/libs/ffmpeg") | ||
| if((CMAKE_GENERATOR_PLATFORM MATCHES "^(aarch64|arm64|ARM64)")) |
There was a problem hiding this comment.
it seems that CMAKE_GENERATOR_PLATFORM is only used by visual studio generator so it might fail with ninja generation. Better to use CMAKE_SYSTEM_PROCESSOR, see
https://cmake.org/cmake/help/latest/variable/CMAKE_GENERATOR_PLATFORM.html
https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_PROCESSOR.html
There is unnecessary double parenthesis
| ${FFMPEG_ROOT}/include | ||
| ${FFMPEG_WIN32_PREBUILT_DIR}/include | ||
| ${PC_FFMPEG_INCLUDE_DIRS} | ||
| /Library/Frameworks |
There was a problem hiding this comment.
it seems that this path wont exist as it is and no binary will be found from here. Can you search in homebrew instead such as /opt/homebrew/
| message(STATUS " libavcodec: ${FFMPEG_LIBAVCODEC_LIBRARIES}") | ||
| message(STATUS " libavformat: ${FFMPEG_LIBAVFORMAT_LIBRARIES}") | ||
| message(STATUS " libavutil: ${FFMPEG_LIBAVUTIL_LIBRARIES}") | ||
| include_directories(${FFMPEG_INCLUDE_DIRS}) |
There was a problem hiding this comment.
this is project wide maybe better to have it at target level with:
target_include_directories
| if(FFMPEG_FOUND) | ||
| message("Found FFMPEG/LibAV libraries") | ||
| include_directories(${FFMPEG_INCLUDE_DIR}) | ||
| message(STATUS " libavcodec: ${FFMPEG_LIBAVCODEC_LIBRARIES}") |
There was a problem hiding this comment.
use FFMPEG_LIBAVCODEC_LIBRARy
| ${FFMPEG_WIN32_PREBUILT_DIR}/lib | ||
| ${PC_FFMPEG_LIBRARY_DIRS} | ||
| /Library/Frameworks | ||
| ~/Library/Frameworks |
There was a problem hiding this comment.
I dont think we gonna find any ffmpeg include dir here
Description
This PR addresses two issues:
When FFmpeg is not found at build time and no codec is explicitly passed at runtime, codecType defaults to VK_VIDEO_CODEC_OPERATION_NONE_KHR. The assert() calls validating this are stripped in release builds, so the demuxer silently proceeds with invalid parameters and fails with a misleading "Video decode queue family not supported" error. See linked issue for more details.
The FindFFmpeg.cmake module had two separate search passes for FFmpeg libraries: one via a custom FFMPEG_FIND macro and another via pkg_check_modules + find_library. It used FFMPEG as the pkg-config prefix, which caused pkg-config to overwrite the module's own output variables
Type of change
bug fix
Issue (optional)
Fixes #185.
Tests
NVIDIA GeForce RTX 4060 Ti / NVIDIA 595.44.00 / Ubuntu 24.04.4 LTS
Total Tests: 73
Passed: 62
Crashed: 0
Failed: 0
Not Supported: 9
Skipped: 2 (in skip list)
Success Rate: 100.0%
AMD Radeon RX 7600 (RADV NAVI33) / radv Mesa 26.1.0-devel (git-055aec542e) / Ubuntu 24.04.4 LTS
Total Tests: 73
Passed: 63
Crashed: 0
Failed: 0
Not Supported: 6
Skipped: 4 (in skip list)
Success Rate: 100.0%
Intel(R) UHD Graphics 770 (ADL-S GT1) / Intel open-source Mesa driver Mesa 26.1.0-devel (git-055aec542e) / Ubuntu 24.04.4 LTS
Total Tests: 73
Passed: 54
Crashed: 0
Failed: 0
Not Supported: 14
Skipped: 5 (in skip list)
Success Rate: 100.0%
AMD Radeon RX 6600 / AMD proprietary driver 26.3.1 (AMD proprietary shader compiler) / Windows 10
Total Tests: 73
Passed: 37
Crashed: 0
Failed: 0
Not Supported: 28
Skipped: 8 (in skip list)
Success Rate: 100.0%